Skip to content

Refactor FXIOS-12796 [Swift 6 Migration] Fix main actor isolation warnings that are Swift 6 errors in the XCUITests suite - Batch 2#2

Open
tomerqodo wants to merge 6 commits into
claude_claude_vs_qodo_base__refactor_fxios-12796_swift_6_migration_fix_main_actor_isolation_warnings_that_are_swift_6_errors_in_the_xcuitests_suite_-_batch_2_pr2from
claude_claude_vs_qodo_head__refactor_fxios-12796_swift_6_migration_fix_main_actor_isolation_warnings_that_are_swift_6_errors_in_the_xcuitests_suite_-_batch_2_pr2
Open

Refactor FXIOS-12796 [Swift 6 Migration] Fix main actor isolation warnings that are Swift 6 errors in the XCUITests suite - Batch 2#2
tomerqodo wants to merge 6 commits into
claude_claude_vs_qodo_base__refactor_fxios-12796_swift_6_migration_fix_main_actor_isolation_warnings_that_are_swift_6_errors_in_the_xcuitests_suite_-_batch_2_pr2from
claude_claude_vs_qodo_head__refactor_fxios-12796_swift_6_migration_fix_main_actor_isolation_warnings_that_are_swift_6_errors_in_the_xcuitests_suite_-_batch_2_pr2

Conversation

@tomerqodo
Copy link
Copy Markdown

Benchmark PR from agentic-review-benchmarks#2

specificForPlatform = .pad
if iPad() {
super.setUp()
await super.setUp()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Bug: IpadOnlyTestCase.setUp() at line 511 calls await super.setUp() without try, but BaseTestCase.setUp() is declared async throws. This is a Swift compile error -- calling a throwing function without try is illegal. The sibling IphoneOnlyTestCase correctly uses try await super.setUp() on line 520; this line should match.

Extended reasoning...

What the bug is

At line 511 of BaseTestCase.swift, the IpadOnlyTestCase.setUp() method calls await super.setUp() without the try keyword. The method itself is declared as override func setUp() async throws, and the parent BaseTestCase.setUp() is also declared as async throws (line 112). In Swift, calling a function that can throw without using try is a compile-time error.

The specific code path

The PR changed IpadOnlyTestCase.setUp() from a synchronous override func setUp() (which called super.setUp()) to an asynchronous override func setUp() async throws. During this conversion, the call was updated from super.setUp() to await super.setUp(), but the try keyword was accidentally omitted. The corresponding IphoneOnlyTestCase class was correctly updated to use try await super.setUp() at line 520.

Why existing code does not prevent it

This is a straightforward oversight during the async/throws migration. The old synchronous setUp() did not need try because it was not declared as throwing. The new version requires both await (for async) and try (for throws), but only await was added for IpadOnlyTestCase.

Impact

This will cause a Swift compile error: "Call can throw but is not marked with try". The entire XCUITests suite will fail to compile, blocking all iPad-specific test cases that inherit from IpadOnlyTestCase (such as DragAndDropTestIpad and MultiWindowTests).

Step-by-step proof

  1. IpadOnlyTestCase.setUp() is declared async throws (line 508)
  2. It calls await super.setUp() (line 511)
  3. super.setUp() resolves to BaseTestCase.setUp() which is declared async throws (line 112)
  4. BaseTestCase.setUp() itself calls try await super.setUp() (line 113), confirming it throws
  5. Swift requires try when calling any function declared with throws
  6. Missing try results in a compile error

Fix

Change line 511 from await super.setUp() to try await super.setUp(), matching the pattern used in IphoneOnlyTestCase at line 520.

Comment on lines 109 to 124
}
}

override func setUp() {
super.setUp()
override func setUp() async throws {
try await super.setUp()
continueAfterFailure = false
setUpApp()
setUpScreenGraph()
}

override func tearDown() {
override func tearDown() async throws {
app.terminate()
super.tearDown()
try await super.tearDown()
}

var skipPlatform: Bool {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 forceRestartApp() (line 139) calls tearDown() and setUp() synchronously, but this PR changed both to async throws overrides. In Swift, sync and async methods are separate overloads, so these calls now resolve to XCTestCase's base class no-ops instead of BaseTestCase's overrides. This makes forceRestartApp() effectively a no-op — it will no longer terminate the app, relaunch it, or rebuild the screen graph. This method is called from BookmarksTests (line 401) and AuthenticationTest (line 52).

Extended reasoning...

What the bug is

The forceRestartApp() method at line 139-142 of BaseTestCase.swift calls tearDown() and setUp() synchronously:

func forceRestartApp() {
    tearDown()
    setUp()
}

This PR changed BaseTestCase.setUp() from override func setUp() to override func setUp() async throws, and similarly tearDown() to override func tearDown() async throws. In Swift, a synchronous method and an async throws method with the same name are separate overloads — they are not the same method. The synchronous calls in forceRestartApp() will resolve to XCTestCase's base class synchronous setUp() and tearDown(), which are no-ops.

Step-by-step proof

  1. Before this PR, BaseTestCase overrode func setUp() (sync). When forceRestartApp() called tearDown() and setUp(), Swift's method resolution found the sync overrides in BaseTestCase, which executed app.terminate() (in tearDown) and setUpApp() / setUpScreenGraph() (in setUp).

  2. After this PR, BaseTestCase only overrides func setUp() async throws and func tearDown() async throws. The sync versions no longer exist in BaseTestCase.

  3. When forceRestartApp() (a non-async function) calls tearDown(), Swift looks for a synchronous tearDown() method. It does NOT find one in BaseTestCase (only async throws version exists), so it resolves to XCTestCase.tearDown(), which is a no-op.

  4. Similarly, the setUp() call resolves to XCTestCase.setUp(), another no-op.

  5. Result: forceRestartApp() does nothing — it doesn't terminate the app, doesn't relaunch it, and doesn't rebuild the screen graph.

Impact

This method is called in at least two test files:

  • BookmarksTests.swift line 401: calls forceRestartApp() to restart the app during a bookmarks link validation test
  • AuthenticationTest.swift line 52: calls forceRestartApp() to verify that BasicAuth login persists after an app restart

In both cases, the tests will silently pass without actually restarting the app, meaning they are no longer testing what they intend to test. The AuthenticationTest in particular is supposed to verify persistence across app restarts, which this regression completely undermines.

How to fix

The simplest fix is to make forceRestartApp() async and await the calls:

func forceRestartApp() async throws {
    try await tearDown()
    try await setUp()
}

Then update callers to use await. Alternatively, inline the actual teardown/setup logic (e.g., app.terminate() + setUpApp() + setUpScreenGraph()) directly in forceRestartApp() without going through the async overrides.

Comment on lines 48 to +49
setUpApp()
setUpExperimentVariables()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Bug: FeatureFlaggedTestSuite.setUp() swapped the call order of setUpApp() and setUpExperimentVariables(). Since setUpApp() calls addLaunchArgument(jsonFileName:featureName:) which force-unwraps jsonFileName and featureName (both String!), and these are only set by subclasses in setUpExperimentVariables(), calling setUpApp() first will crash on nil force-unwrap. Restore the original order: call setUpExperimentVariables() before setUpApp().

Extended reasoning...

What the bug is

In FeatureFlaggedTestSuite.setUp() (line 48-49), the PR swapped the call order of setUpApp() and setUpExperimentVariables(). The original code called setUpExperimentVariables() first, then setUpApp(). The new code calls setUpApp() first, then setUpExperimentVariables().

Why this causes a crash

FeatureFlaggedTestSuite declares two properties as implicitly unwrapped optionals:

var jsonFileName: String!
var featureName: String!

These are initialized to nil by default. Subclasses override setUpExperimentVariables() to set these values. Then setUpApp() is supposed to consume them:

override func setUpApp() {
    addLaunchArgument(jsonFileName: jsonFileName, featureName: featureName)
}

The addLaunchArgument method signature takes non-optional String parameters:

func addLaunchArgument(jsonFileName: String, featureName: String)

When String! values that are nil are passed to a function expecting String, Swift performs an implicit force-unwrap. This triggers a fatal error at runtime.

Step-by-step proof

  1. A test class like ActivityStreamTest extends FeatureFlaggedTestBase (not FeatureFlaggedTestSuite), so it is unaffected.
  2. However, any test class that extends FeatureFlaggedTestSuite and overrides setUpExperimentVariables() to set jsonFileName and featureName will hit this bug.
  3. When setUp() runs, continueAfterFailure is set to false.
  4. setUpApp() is called — this calls addLaunchArgument(jsonFileName: jsonFileName, featureName: featureName).
  5. At this point, setUpExperimentVariables() has NOT been called yet, so jsonFileName is nil and featureName is nil.
  6. Passing nil implicitly unwrapped optionals to non-optional String parameters triggers a force-unwrap crash.
  7. The test suite crashes before any test can run.

Impact

Every test class that extends FeatureFlaggedTestSuite will crash during setUp(), meaning none of those tests can execute. This is a complete blocker for any feature-flagged test suite.

Fix

Swap lines 48 and 49 back to the original order:

setUpExperimentVariables()
setUpApp()

Comment on lines 291 to +294
// This DDBB contains those 4 websites listed in the name
let historyAndBookmarksDB = "browserYoutubeTwitterMozillaExample-places.db"

override func setUp() {
override func setUp() async throws {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Bug: DragAndDropTests.tearDown() (line 28) was not converted to async throws like every other tearDown override in this PR. Since BaseTestCase.tearDown() is now async throws, the synchronous super.tearDown() call bypasses BaseTestCase entirely and calls XCTestCase.tearDown() instead, meaning app.terminate() from BaseTestCase.tearDown() may not execute properly for these tests. The inner class DragAndDropTestIpad (line 310) was correctly converted — this is just a missed conversion on the outer class.

Extended reasoning...

What the bug is

The PR converts setUp and tearDown overrides across the XCUITests suite from synchronous to async throws to match the new BaseTestCase signatures. However, DragAndDropTests.tearDown() at line 28 was missed — it still uses the synchronous override func tearDown() pattern with a synchronous super.tearDown() call.

The specific code path

At line 28-31 of DragAndDropTests.swift:

override func tearDown() {
    XCUIDevice.shared.orientation = UIDeviceOrientation.portrait
    super.tearDown()
}

Meanwhile, BaseTestCase.tearDown() (line 125-128 of BaseTestCase.swift) was converted to:

override func tearDown() async throws {
    app.terminate()
    try await super.tearDown()
}

Why existing code doesn't prevent it

When a class hierarchy has both sync and async versions of tearDown, XCTest's behavior becomes nuanced. The synchronous super.tearDown() in DragAndDropTests resolves to XCTestCase.tearDown() (the sync version), not BaseTestCase.tearDown() async throws. This means the synchronous DragAndDropTests.tearDown() code — specifically setting orientation to portrait — may not execute in the expected order relative to the async tearDown chain, or may not execute at all depending on how XCTest dispatches async vs sync tearDown methods.

Step-by-step proof

  1. BaseTestCase.tearDown() is now declared as override func tearDown() async throws
  2. DragAndDropTests inherits from BaseTestCase
  3. DragAndDropTests.tearDown() is declared as override func tearDown() (synchronous)
  4. When XCTest tears down a DragAndDropTests instance, it sees both sync and async tearDown in the hierarchy
  5. The synchronous super.tearDown() call dispatches to XCTestCase.tearDown() (sync), NOT to BaseTestCase.tearDown() async throws
  6. XCTest independently calls the async tearDown chain (BaseTestCase.tearDown), so app.terminate() still runs, but the orientation reset in DragAndDropTests.tearDown() may not execute at all — XCTest only calls the async version when both exist in the hierarchy
  7. This contradicts what the verifiers noted: the orientation reset (XCUIDevice.shared.orientation = .portrait) is the code that will be SKIPPED, not app.terminate()

Impact

The device orientation will not be properly reset to portrait between DragAndDropTests test cases, potentially causing layout-dependent test failures in subsequent tests. Additionally, this leaves a synchronous tearDown that the PR is specifically trying to migrate away from, defeating the purpose of the Swift 6 migration for this class. Every other test class with a tearDown override was correctly converted.

How to fix

Convert the method signature to match the pattern used everywhere else in this PR:

override func tearDown() async throws {
    XCUIDevice.shared.orientation = UIDeviceOrientation.portrait
    try await super.tearDown()
}

This is the exact same pattern already correctly applied to the inner DragAndDropTestIpad class at lines 310-313 of the same file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants